Fix SQLite migration compatibility and idempotency issues - #1006
Fix SQLite migration compatibility and idempotency issues#1006Bornunique911 wants to merge 21 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe migrations conditionally add and remove nullable metadata and embedding columns. They also conditionally create artifact ingestion tables, repair unique constraints, and drop tables in dependency order. ChangesMigration persistence changes
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py (1)
11-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
sa.inspect(conn)instead ofInspector.from_engine(conn).The unpinned runtime SQLAlchemy dependency can resolve to SQLAlchemy 2.x, where
Inspector.from_engine()is deprecated. Replace the duplicated calls in the referenced migrations with the supported inspection entry point.Proposed change
-from sqlalchemy.engine.reflection import Inspector ... - inspector = Inspector.from_engine(conn) + inspector = sa.inspect(conn)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py` around lines 11 - 21, Replace the deprecated Inspector.from_engine(conn) usage with sa.inspect(conn) in the column_exists helper in migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py (lines 11-21), migrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.py (lines 11-21), and migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py (lines 11-21). Preserve the existing column inspection behavior and remove any now-unused Inspector imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py`:
- Around line 39-43: The downgrade in
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py at
lines 39-43 must remove document_metadata from both cre and node using the
existing SQLite batch-table migration pattern, replacing the no-op downgrade.
Apply the same reversible downgrade change in
migrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.py at
lines 31-33 to remove embedding_vec from embeddings, and add downgrade coverage
for both revisions.
In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py`:
- Around line 24-71: Update the migration logic around artifact_ingest_event and
ingest_chunk so existing tables are inspected for
uq_artifact_ingest_event_run_artifact and uq_ingest_chunk_artifact_chunk. When
either constraint is missing, rebuild or otherwise alter the table to add it
before the migration completes; do not silently skip DDL, and fail the migration
if the constraint cannot be repaired.
---
Nitpick comments:
In `@migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py`:
- Around line 11-21: Replace the deprecated Inspector.from_engine(conn) usage
with sa.inspect(conn) in the column_exists helper in
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py (lines
11-21), migrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.py
(lines 11-21), and
migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py (lines
11-21). Preserve the existing column inspection behavior and remove any
now-unused Inspector imports.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b0c9beb-ffbf-47a6-a28f-116a34aae300
📒 Files selected for processing (3)
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.pymigrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.pymigrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py
…ints - Add downgrades to 055dbd9f8bfe and 967016ee10fa to drop added columns - Replace deprecated Inspector.from_engine(conn) with sa.inspect(conn) - In 9f1a2b3c4d5e, verify that existing tables have the required unique constraints; add them via batch_alter_table if missing - Ensure migration fails if constraints cannot be added Addresses PR review comments OWASP#1006
Before change on the main website opencre.org/node/standard/CWE :
After change locally on 127.0.0.1:5000/node/standard/CWE :
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py (1)
94-97: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTrack table ownership before dropping in
downgrade().
upgrade()may only add the missing unique constraints to existingartifact_ingest_eventoringest_chunktables, whiledowngrade()always runsop.drop_table()for both. If a pre-existing table was adopted instead of created here, downgrade destroys its rows and schema. Track ownership or downgrade only constraint-alterations made by this revision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py` around lines 94 - 97, Update the migration’s upgrade/downgrade flow to track whether artifact_ingest_event and ingest_chunk were created by this revision versus merely adopted with added constraints. In downgrade(), drop only tables owned and created by this revision; for pre-existing tables, revert only the unique constraints added by this migration and preserve their rows and schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py`:
- Around line 38-43: Align downgrade ownership checks with the conditional
upgrades: in
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py lines
38-43, update downgrade() to drop document_metadata from cre and node only when
each column exists, preserving pre-existing columns; in
migrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.py lines
30-33, apply the same existence check before dropping embedding_vec from
embeddings. Add downgrade coverage for absent columns and populated pre-existing
columns at both sites.
In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py`:
- Around line 57-61: Before the artifact_ingest_event batch rewrite and
create_unique_constraint operation, handle the ingest_chunk foreign key
explicitly: temporarily enable foreign-key enforcement, drop the child
constraint, and recreate it with its existing cascade behavior after the
rewrite. Add a regression test covering pre-existing artifact_ingest_event rows
and verifying the ingest_chunk relationship remains valid.
- Around line 57-61: Update the existing-table migration paths using
op.batch_alter_table for artifact_ingest_event and ingest_chunk so unnamed
UNIQUE constraints are preserved during SQLite table recreation. Copy or replace
each supported unnamed uniqueness rule before adding the named constraint,
reject unsupported legacy schemas, or use recreate="always" with explicit
table_args; ensure no existing uniqueness rule is silently dropped.
- Around line 22-26: Update constraint_exists to validate both the constraint
name and its column_names against the migration’s expected target columns,
returning true only for an exact definition match; if the name exists with
different columns, do not treat it as present so the migration can correct it.
Apply the same validation at the checks around the constraints named
uq_artifact_ingest_event_run_artifact and uq_ingest_chunk_artifact_chunk.
---
Outside diff comments:
In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py`:
- Around line 94-97: Update the migration’s upgrade/downgrade flow to track
whether artifact_ingest_event and ingest_chunk were created by this revision
versus merely adopted with added constraints. In downgrade(), drop only tables
owned and created by this revision; for pre-existing tables, revert only the
unique constraints added by this migration and preserve their rows and schema.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d32bd9f-c55b-4fa9-8cb2-af49b08dfa50
📒 Files selected for processing (3)
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.pymigrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.pymigrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Thanks for the SQLite migration work. Please wait for #995 to land first, then rebase this PR onto Why #995 firstBoth PRs add Fixes after rebase
Happy to re-review once rebased on #995. |
Thanks for the review – that makes sense. I'll wait for #995 to merge, then rebase this PR onto main. After rebasing, I'll:
Once I've rebased and cleaned up, I'll re‑request your review. Thanks for the guidance! |
#995 is rebased please check @northdpole ! |
|
#995 has merged. Please rebase this PR onto latest |
…ints - Add downgrades to 055dbd9f8bfe and 967016ee10fa to drop added columns - Replace deprecated Inspector.from_engine(conn) with sa.inspect(conn) - In 9f1a2b3c4d5e, verify that existing tables have the required unique constraints; add them via batch_alter_table if missing - Ensure migration fails if constraints cannot be added Addresses PR review comments OWASP#1006
c79da22 to
d4bea6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py (1)
32-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
print()calls from the migration.The PR objectives explicitly require migrations without
print()statements. Remove the four unconditional stdout writes. Keep migration diagnostics in the project’s standard logging path if diagnostics are required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py` around lines 32 - 41, Remove all four print() calls from the migration’s column-existence branches, including the messages for the cre and node tables. Preserve the existing column checks and op.add_column operations; use the project’s standard logging path only if migration diagnostics are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py`:
- Around line 44-58: The downgrade function must not drop pre-existing
document_metadata columns, since column_exists only confirms presence, not
ownership. Make the migration ownership-aware by folding this revision into the
original `#995` migration or recording which columns upgrade() actually added,
then have downgrade() drop only those columns; add regression coverage for both
cre and node.
- Around line 31-38: Update the migration’s document_metadata column definitions
for both cre and node to use sa.JSON() instead of sa.Text(), matching the JSON
handling in the application. Remove the related migration print() calls while
preserving the existing column-existence checks and conditional additions.
---
Nitpick comments:
In
`@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py`:
- Around line 32-41: Remove all four print() calls from the migration’s
column-existence branches, including the messages for the cre and node tables.
Preserve the existing column checks and op.add_column operations; use the
project’s standard logging path only if migration diagnostics are required.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: e67e0d87-a4a7-4297-8073-46ddaafd26af
📒 Files selected for processing (1)
migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Review — SQLite migration compatibility (#1006)
Thanks for the focused migration PR (right call per the Slack priority — keep this separate from the importer stack). CI is green. A few things need fixing before merge.
Blocker — _migration_tracking schema clash
9f1a2b3c4d5e creates _migration_tracking as (revision, table_name) PK.
b5ac48010165 (later in the same chain) assumes (revision, table_name, column_name) and only create_tables when the table is missing.
On a fresh flask db upgrade / make migrate-upgrade:
9f1acreates the 2-column tracking tableb5acsees the table already exists → skips createINSERT (... column_name)into the 2-column table → fails
Please unify on one tracking schema (or drop the custom tracking table entirely — see below).
Blocker / design — rewriting already-shipped revisions
This PR edits 9f1a… and b5ac…, which are already on main and may already be stamped in local/prod DBs.
Alembic will not re-run those upgrades. So:
- DBs that already stamped the old
9f1a(or failed after stamp) do not get the SQLite unique-constraint fix from this edit - Changing
b5ac’s downgrade from the intentional no-op (#995 / prod safety) to “drop if tracked” changes the shipped contract for environments that never had tracking rows
Preferred pattern: leave shipped migrations alone; add a new forward revision (or small set) off current head b5ac48010165 that:
- Makes artifact ingest tables / unique constraints idempotent for SQLite (batch_alter / create-if-missing)
- Adds SQLite
embeddings.embedding_vecTEXT if missing (your967016ee10fais the right shape — keep it as a new head, don’t rely on rewritingb5ac) - Avoids inventing
_migration_trackingunless we really need downgrade archaeology — for SQLite/dev,IF NOT EXISTS/ inspector checks are enough; for prod, prefer the #995 no-op downgrade style fordocument_metadata
What looks good
- Putting
UniqueConstraintinsidecreate_table(fixes “No support for ALTER of constraints in SQLite”) - Idempotent
column_exists/table_existschecks - New
967016ee10faextendingb5ackeeps a single Alembic head (good — verified against currentmain) - No
print()left in upgrades
Also
- Please rebase onto latest
main(~11 behind; #990/#991 landed). - #950 is already merged — no need to wait on it after this.
- Hold further CWE/importer PRs until this lands (per maintainer priority).
Happy to re-review quickly once tracking is unified (or removed) and the SQLite fixes are in new revisions rather than edits to shipped ones.
|
@Bornunique911 thanks for the Slack ping — reviewing now. Not merge-ready yet: see the review for the Please rebase on latest |
|
@Bornunique911 thank you for the focused migration work and for chasing the CWE / SQLite follow-ups — the split-out PR was the right call, and the diagnosis ( We're going to reshape this PR slightly on your branch (maintainer edits): keep the SQLite-safe unique constraints, leave the shipped |
…ints - Add downgrades to 055dbd9f8bfe and 967016ee10fa to drop added columns - Replace deprecated Inspector.from_engine(conn) with sa.inspect(conn) - In 9f1a2b3c4d5e, verify that existing tables have the required unique constraints; add them via batch_alter_table if missing - Ensure migration fails if constraints cannot be added Addresses PR review comments OWASP#1006
…ta column in downgrade
…logic for document_metadata column
Keep UniqueConstraint inside create_table for SQLite, drop the custom _migration_tracking helper (schema clash risk), restore the OWASP#995 document_metadata no-op downgrade contract, and limit embedding_vec downgrade to SQLite only.
4bc02ef to
2a91518
Compare
|
Maintainer reshape pushed to this branch (rebased on latest
Waiting on CI — will re-review/approve when green. |
3 participants
Problem
Several recent migrations were not fully compatible with SQLite, causing errors when developers run
make migrate-upgradeor./scripts/update-cwe.sh:op.create_unique_constraint()afterCREATE TABLEfails withNo support for ALTER of constraints in SQLite dialect.Missing
document_metadatacolumn increandnodetables causesno such columnerrors.Missing
embedding_veccolumn inembeddingstable causesno such column: embeddings.embedding_vec.Some migrations are not idempotent, causing
table already existserrors on re-runs.Solution
9f1a2b3c4d5e– DefineUniqueConstraintinsideCREATE TABLE;addtable_existsguards.055dbd9f8bfe– Adddocument_metadatawith column existence checks (new migration).967016ee10fa– Addembedding_vecasTEXTfor SQLite with existence check (new migration).Testing
make migrate-upgrade→make upstream-sync→./scripts/update-cwe.shall succeed.✅ Existing database: re-runs are idempotent, skip already-created objects.
✅ No regressions for PostgreSQL.
Impact
Developers using SQLite can now run migrations and import data without manual workarounds.
Makes the project more contributor‑friendly for SQLite users.
Ready for review. Let me know if any adjustments are needed.